-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix media uploads for Docker in rootless mode #50608
Conversation
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
I managed to get e2e tests working in rootless docker with this change on my machine, but I'm getting this error on GitHub doing the same thing.
@noahtallen I'm no Docker expert, is there any reason this would be failing here? Or is there a better way to change permissions in Docker? |
Hm, I'm no expert either, but throwing out some ideas:
I'm also curious, what errors do you see in rootless mode? I also don't have a good way to test rootless mode unfortunately. thanks for opening a PR! |
3886006
to
71d97f6
Compare
The DOCKERFILE is running the commands as root, so chown is necessary to move ownership to the host user.
That's what I started with, but I kept getting the same error until I created the dir and chowned it.
The
Updated the description. I should have had that added before :) |
Flaky tests detected in c44b630. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5049246189
|
I made it past |
8d9cf28
to
fe62240
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting @ajlende. Given the existence of this particular problem, however, I think there will be other permission problems when running Docker in rootless mode to consider. All of the issues fixed by #49962 would be experienced in rootless mode.
The problem is that the uid
and gid
in rootless mode are in the host user's namespace rather than being mapped in the same way. This means that root
in the container is actually the user's uid
and your host user's uid
in the container is an offset from the host user's uid
. The end result is that the "user" in the container doesn't have write access to any of the files.
This PR works because you're forcing ownership of /var/www/html/wp-content/uploads
to the user namespace uid
, but, I wonder if there's another approach where we can preserve the fixes provided by the original PR? What if we set the userns_mode
to host
in the docker-compose.yml
file? The Docker daemon would still not be running as root, but, the uid
mapping should work again, right? This would also only apply to the wp-env
containers.
@@ -54,6 +54,11 @@ function getMounts( | |||
? `user-home:/home/${ hostUsername }` | |||
: `tests-user-home:/home/${ hostUsername }`; | |||
|
|||
const userUploadsMount = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there is a mappings
entry for /var/www/html/wp-content/uploads
already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I don't really know that much about docker so I may be misunderstanding. This function creates a new Set()
which, by definition, doesn't contain duplicates, so I don't think there could already be an entry for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take, for example, a wp-env.json
file containing this:
{
"mappings": {
"/var/www/html/wp-content/uploads": "./env/uploads"
}
}
This would translate to a unique entry in the Set, but, be a duplicate mapping for /var/www/html/wp-content/upload
.
The tests are all consistently passing locally, but consistently failing on GitHub. I'm not really sure how to debug this. |
Also, #32519 should actually already be fixed. The |
Thanks for the quick review @ObliviousHarmony!
I just tried that on a fresh branch off of trunk, and I still end up with the original error where uploads aren't allowed.
I was wondering if we should set up permissions as described in the Hardening WordPress docs, but I had something working locally with just the uploads change and figured it would be better to start small and fix my one issue first. |
We did some further debugging together and weren't able to find a way to make this work. I did some further research and it looks like this is unfortunately unavoidable. If you're running in rootless then using your host
Honestly, I would prefer if we avoided added any permission hacks specific for Rootless Docker. There's a problem here in how permissions work between these two deployments, but, I've got an idea! In #50814 (comment) it was highlighted that there's a problem with Windows caused by the usage of |
Let me take a pass at solving both of these problems in one go. Could you share the output of |
[
{
"Name": "default",
"Metadata": {},
"Endpoints": {
"docker": {
"Host": "unix:///run/user/1000/docker.sock",
"SkipTLSVerify": false
}
},
"TLSMaterial": {},
"Storage": {
"MetadataPath": "\u003cIN MEMORY\u003e",
"TLSPath": "\u003cIN MEMORY\u003e"
}
}
] |
What about |
Nice, I love the idea of solving both problems at once! |
It does seem to list |
I ran into what should have been a pretty obvious problem with running everything as I'll let @noahtallen weigh in, but, I don't think rootless Docker is something we should try and go to extreme lengths to support. Ultimately it boils down to everything being owned by Ideally |
I mostly agree with @ObliviousHarmony. I don't think |
Thanks for the help and teaching me some things about wp-env. Since this isn't as simple as I first thought, we'll close this PR, and I'll take @ObliviousHarmony's suggestion to just modify my containers manually for now. |
What?
Fixes media uploads for Docker running in rootless mode.
Why?
Running Docker in rootless mode provides an extra layer of security by not running the daemon with the root user.
May also solve #32519 because it's using a volume for the uploads now.
How?
Testing Instructions
wp-env start
Testing Instructions for Keyboard
Screenshots or screencast